test: add RDC_DATA_DIR seam and centralize unit/e2e home isolation#251
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Warning Review limit reached
More reviews will be available in 43 minutes and 41 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesRDC_DATA_DIR env override and centralized test isolation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/conftest.py (1)
40-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding logging to
_reap_daemonsfor observability.The daemon cleanup is a critical part of test isolation. Adding log messages when a daemon is successfully terminated (or termination fails) would improve debugging when cleanup doesn't work as expected.
📊 Proposed logging additions
def _reap_daemons(data_dir: Path) -> None: """Terminate any daemons still recorded under *data_dir* (best effort).""" for session_file in data_dir.glob("sessions/*.json"): try: pid = int(json.loads(session_file.read_text()).get("pid", 0)) except (OSError, ValueError, TypeError, json.JSONDecodeError): continue if pid > 0 and _platform.is_pid_alive(pid): - _platform.terminate_process_tree(pid) + if _platform.terminate_process_tree(pid): + _log.info("Terminated daemon PID %d from session %s", pid, session_file.name) + else: + _log.warning("Failed to terminate daemon PID %d from session %s", pid, session_file.name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/conftest.py` around lines 40 - 48, Add logging to the _reap_daemons function to improve observability of daemon cleanup operations. Include log messages when a daemon process is successfully terminated via _platform.terminate_process_tree(pid), and also add logging when termination fails or encounters errors. This will help with debugging when test cleanup doesn't work as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/e2e/conftest.py`:
- Around line 40-48: Add logging to the _reap_daemons function to improve
observability of daemon cleanup operations. Include log messages when a daemon
process is successfully terminated via _platform.terminate_process_tree(pid),
and also add logging when termination fails or encounters errors. This will help
with debugging when test cleanup doesn't work as expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7e893dd-a00f-424e-b5a2-c7feea28482a
📒 Files selected for processing (20)
src/rdc/_platform.pytests/e2e/conftest.pytests/e2e/e2e_helpers.pytests/unit/conftest.pytests/unit/test_android_commands.pytests/unit/test_capture_control.pytests/unit/test_cli_session_flag.pytests/unit/test_keep_remote.pytests/unit/test_platform.pytests/unit/test_remote_commands.pytests/unit/test_remote_core.pytests/unit/test_remote_setup.pytests/unit/test_remote_state.pytests/unit/test_remote_status_disconnect.pytests/unit/test_session_commands.pytests/unit/test_session_service.pytests/unit/test_session_state.pytests/unit/test_shader_preload.pytests/unit/test_split_core.pytests/unit/test_target_state.py
💤 Files with no reviewable changes (13)
- tests/unit/test_remote_commands.py
- tests/unit/test_remote_state.py
- tests/unit/test_keep_remote.py
- tests/unit/test_android_commands.py
- tests/unit/test_split_core.py
- tests/unit/test_shader_preload.py
- tests/unit/test_remote_status_disconnect.py
- tests/unit/test_remote_setup.py
- tests/unit/test_cli_session_flag.py
- tests/unit/test_session_state.py
- tests/unit/test_session_service.py
- tests/unit/test_capture_control.py
- tests/unit/test_target_state.py
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
1 participant
_platform.data_dir()had no env override, so test isolation was 33 hand-copiedmonkeypatchseams across 14 unit files, and the e2e suite ran the real CLI against the developer's real~/.rdc(leaking sessions and live daemons on failed teardown).RDC_DATA_DIRoverride todata_dir()(production default unchanged; renderdoc-lib discovery untouched).tests/unit/conftest.py."localhost" not in str(path)assertion intest_remote_core.pythat broke once the data dir moved under a tmp path whose node-id contains "localhost" (the exact filename assertion is kept).Regression tests fail without the seam (verified). Unit suite 2995 passed.
Summary by CodeRabbit
RDC_DATA_DIRenvironment variable. When set, this variable overrides platform-specific default locations. If unset or empty, the system falls back to the default directory, maintaining backward compatibility.